[FLINK-38647] Support changelog-mode=upsert + null-tolerance#4437
[FLINK-38647] Support changelog-mode=upsert + null-tolerance#4437cansakiroglu wants to merge 3 commits into
Conversation
FLINK-38647 reports a NullPointerException from the Postgres Pipeline source
when a captured table has REPLICA IDENTITY DEFAULT and an UPDATE arrives that
does not change the primary key. Root cause: PostgresDataSource hardcodes
DebeziumChangelogMode.ALL, which makes the deserializer extract a before-image
that is null under DEFAULT replication, NPE'ing in
DebeziumSchemaDataTypeInference.inferStruct.
This change fixes the issue from two angles:
1. Connector side — expose 'changelog-mode' YAML option on the Postgres
Pipeline connector (mirrors the legacy SQL connector). Accepts 'all'
(default, current behaviour) or 'upsert'. In upsert mode the source emits
UPDATE events with before == null and only after populated, so the
pipeline runs cleanly under REPLICA IDENTITY DEFAULT without requiring
FULL (which roughly multiplies WAL volume per UPDATE by column count).
2. Runtime side — make DataChangeEventSerializer null-tolerant. Three changes:
- copy() null-guards each recordDataSerializer.copy() call so the chained
CopyingChainingOutput.pushToOperator path stops NPE'ing on null
before/after.
- serialize() writes 2 leading boolean presence flags before conditionally
writing each record. Required because the previous serialize() already
skipped writing null fields, but deserialize() always tried to read two
records back — the wire format itself couldn't roundtrip a null-before
UPDATE.
- deserialize() reads the 2 flag bytes and skips the corresponding read
when absent.
Wire format change is symmetric but breaking vs current 3.6.x. Production
checkpoints/savepoints written by older versions cannot be restored by the
new code without a snapshot-version-aware deserialize path; see the PR
description for the proposed compat strategy (bumping
DataChangeEventSerializerSnapshot.CURRENT_VERSION to 2 and reading old
format when version == 1). Happy to add that in a follow-up commit on this
PR — wanted reviewers' opinion on the strategy first.
Signed-off-by: Mehmet Can Şakiroğlu <cansakiroglu@gmail.com>
| @Experimental | ||
| public static final ConfigOption<String> CHANGELOG_MODE = | ||
| ConfigOptions.key("changelog-mode") | ||
| .stringType() | ||
| .defaultValue("all") |
There was a problem hiding this comment.
I noticed the existing SQL connector (flink-connector-postgres-cdc) defines this option using enumType(DebeziumChangelogMode.class). It might be worth aligning with that approach — it would give us compile-time safety and also avoid the null handling concern in the factory. Something like:
public static final ConfigOption<DebeziumChangelogMode> CHANGELOG_MODE =
ConfigOptions.key("changelog-mode")
.enumType(DebeziumChangelogMode.class)
.defaultValue(DebeziumChangelogMode.ALL);
| String changelogModeRaw = config.get(CHANGELOG_MODE); | ||
| DebeziumChangelogMode changelogMode; | ||
| switch (changelogModeRaw.toLowerCase()) { |
There was a problem hiding this comment.
If enumType is adopted for CHANGELOG_MODE, this switch block could be simplified to a single config.get(CHANGELOG_MODE) call.
Also, it might be good to add a primary key validation for upsert mode, similar to what the SQL connector does in PostgreSQLTableFactory.
| target.writeBoolean(beforePresent); | ||
| target.writeBoolean(afterPresent); |
There was a problem hiding this comment.
This looks like a wire format breaking change — old deserializers would interpret these 2 boolean bytes as the start of the before RecordData, which could cause stream corruption on checkpoint restore.Would it be possible to bump DataChangeEventSerializerSnapshot.CURRENT_VERSION to 2 in this PR and add a version-aware fallback?
I think this would be important to include before merging, since checkpoint restoration is a production-critical path.
|
Nice PR! I've left a few inline comments with suggestions on the implementation. It would also be great to have some test coverage — mainly roundtrip tests for the serializer with null before/after, and a basic unit test. Looking forward to the updates! |
…n, versioned deserialize, tests - PostgresDataSourceOptions: define 'changelog-mode' via enumType(DebeziumChangelogMode.class), aligning with the SQL connector; invalid values now fail at option validation. - PostgresDataSourceFactory: drop the string switch; validate that all captured tables have a primary key when changelog-mode=upsert, mirroring PostgreSQLTableFactory. - DataChangeEventSerializer: introduce CURRENT_VERSION = 2 and a version-aware deserialize(int, DataInputView) that still reads the pre-flags op-dependent layout, following the PhysicalColumnSerializer/SchemaSerializer convention. DataChangeEvent is never persisted in checkpoint state, so the snapshot stays a SimpleTypeSerializerSnapshot; the versioned overload keeps the old layout readable if event bytes are ever embedded in versioned state. - Tests: null-before UPDATE events in DataChangeEventSerializerTest test data (also exercised via EventSerializerTest), legacy-version and unknown-version deserialize tests, an upsert-mode roundtrip test, and factory tests for option parsing and PK validation.
|
Thank you for this PR. This PR resolves the source part, but the sink part is still broken. Case:
The reason is that a PostgreSQL upsert event does not have We verified a one-line guard end-to-end (PG -> Paimon, DML + schema evolution): case UPDATE:
if (hasPrimaryKey && dataChangeEvent.before() != null) { // skip UPDATE_BEFORE when there is nothing to build it from
...For primary-key tables this is safe: Note: the Kafka sink has the same pattern — Would you prefer to include the sink-side null-tolerance in this PR, or should we file a separate JIRA as a follow-up to FLINK-39684? |
|
Thanks Donghwan for the kind review. I believe it's incorrect for the PostgreSQL source to emit partial UpdateEvents that leave the before field I wonder if we can emit the |
|
Thanks @yuxiqian, that makes sense — emitting Agreed this is the cleaner approach: We implemented this and verified it with a real pipeline run on Docker (Flink 2.2.1, PostgreSQL 15 with @cansakiroglu I've opened a PR against your branch with this change as a single commit: cansakiroglu#1 |
… null-before UPDATE events (#1) An UPDATE event with a null before image breaks the implicit contract that runtime operators and sinks rely on. Instead of making them null-tolerant, the source now emits REPLACE events (which carry the after image only) in upsert changelog mode. REPLACE is already handled by the event serializer and all pipeline sinks, so the null-tolerant DataChangeEventSerializer changes are reverted and no sink changes are needed.
FLINK-38647 reports a NullPointerException from the Postgres Pipeline source when a captured table has REPLICA IDENTITY DEFAULT and an UPDATE arrives. Root cause: PostgresDataSource hardcodes DebeziumChangelogMode.ALL, which makes the deserializer extract a before-image that is null under DEFAULT replication, hence NPE'ing.
This change fixes the issue from two angles:
Connector side: expose 'changelog-mode' YAML option on the Postgres Pipeline connector. Accepts 'all' (default, current behaviour) or 'upsert'. In upsert mode the source emits UPDATE events with before == null and only after populated, so the pipeline runs cleanly under REPLICA IDENTITY DEFAULT without requiring FULL (which increases WAL volume).
Runtime side: make serializer null-tolerant. Three changes: